Skip to content

fix: only start one hotplug watcher, so connect fires once per plug - #36

Open
TKDubsta wants to merge 2 commits into
node-usb:mainfrom
TKDubsta:fix/duplicate-hotplug-watcher
Open

fix: only start one hotplug watcher, so connect fires once per plug#36
TKDubsta wants to merge 2 commits into
node-usb:mainfrom
TKDubsta:fix/duplicate-hotplug-watcher

Conversation

@TKDubsta

Copy link
Copy Markdown

fix: only start one hotplug watcher, so connect fires once per plug

I ran into this moving an Electron app over from node-usb 2.x: a single plug-in was
triggering our USB alert rules twice. connect fires twice for one physical plug on
3.0.1.

start_watching checks whether a watcher is already running and then stores the new
one, but lets go of the lock in between. addEventListener kicks off addAttach and
addDetach back to back without awaiting either, so they run at the same time, both
see an empty slot, and both spawn a watcher. Two watchers, so every hotplug event gets
delivered twice.

disconnect only looks fine by accident — the TS side checks knownDevices.has(handle)
and deletes the handle as it fires, so the duplicate gets swallowed there. connect has
no equivalent check, which is why you get two connects but one disconnect.

Looks like it arrived with the watch_task refactor in #18. 3.0.0 did this a different
way and isn't affected.

Reproducing it

You don't need a device for this — the race is on registration, not on the plug. Drop an
eprintln!("spawned") immediately before the tokio::spawn in start_watching, then:

const { webusb } = require('./dist');
// the first listener of either type kicks off both addAttach and addDetach
webusb.addEventListener('connect', () => {});
setTimeout(() => process.exit(0), 750);

On main that prints twice, every time — 50 out of 50 runs here, always from two
different threads. With the patch it prints once, 50 out of 50.

On actual hardware, replugging a Brother PT-P950NW on 3.0.1 gave 1 disconnect and 2
connects (same tick, same serial), against 1 and 1 on node-usb 2.18.0.

The fix

Hold one lock across both the check and the store, which means putting watch_task
behind an Arc<Mutex<..>> the way callbacks already is.

Ive also taken the four napi methods from &mut self to &self. napi can poll them
concurrently, so &mut self was handing out aliasing &mut — the duplicate watcher is
really just the visible symptom of that. They don't need to be unsafe any more either,
which clears four clippy warnings. Neither start_watching nor stop_watching has an
await in it now, so both drop async.

I checked it doesn't over-correct: remove both listeners and re-register, and you get
exactly one fresh watcher, so teardown still works. Generated index.d.ts is unchanged,
and fmt, build and tsc all pass.

Things you might want done differently

This could be fixed in the TS instead, by chaining the two native calls so they can't
overlap — probably a two-line change. I went the Rust route because the gap is open to
any concurrent caller rather than just that one call site, and the &mut self seemed
worth getting rid of regardless. Happy to redo it the other way if you'd rather.

The lock is now held across nusb::watch_devices(). It's only contended while listeners
are being set up, so I don't think it matters in practice, but if you'd sooner not hold
it there, moving the stream creation inside the spawned task would narrow it to just the
store.

Only built and run on macOS arm64.

start_watching checked whether a watcher was live and then stored a new
one, releasing the lock in between. addEventListener starts addAttach and
addDetach back-to-back without awaiting, so both ran at once, both saw an
empty slot, and each spawned a watcher. Two watchers delivered every
hotplug event twice, which surfaced as connect firing twice for a single
plug-in. disconnect was unaffected because the JS side already dedupes it.

Hold one lock across the check and the store, and take the napi methods
from &mut self to &self: napi can poll them concurrently, so &mut self was
handing out aliasing mutable references. They no longer need to be unsafe.

Regression from the watch_task refactor in node-usb#18; v3.0.0 is unaffected.
Copilot AI review requested due to automatic review settings July 31, 2026 23:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a concurrency race in the Rust-side hotplug watcher management so that a single physical USB plug-in results in exactly one connect event, aligning behavior with node-usb 2.x expectations and preventing duplicate event delivery in consumers (e.g., Electron apps).

Changes:

  • Makes watch_task shared/mutable via Arc<Mutex<Option<JoinHandle<()>>>> and holds a single lock across the “is watcher running?” check and storing the spawned task to prevent double-spawn.
  • Removes unnecessary unsafe, switches N-API methods from &mut self to &self, and makes start_watching / stop_watching synchronous since they no longer await.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/lib.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants